Calculate file options for WheelWriter once and cache the result#2865
Calculate file options for WheelWriter once and cache the result#2865
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the WheelWriter to cache file options (compression settings and modification time) at initialization rather than recalculating them for each file operation. The mtime() method is removed from WheelWriter and replaced with a new zip_mtime() function in BuildContext that is called once when creating the writer.
- Moved SOURCE_DATE_EPOCH handling from
WheelWriter::mtime()toBuildContext::zip_mtime() - Changed
WheelWriterto storeSimpleFileOptionsinstead ofCompressionOptions - Updated lifetime annotation in
CompressionOptions::get_file_options()from'_to'static
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/module_writer/wheel_writer.rs | Removed mtime() method; changed field from compression: CompressionOptions to file_options: SimpleFileOptions; simplified add_bytes() and finish() to use cached options; updated test to call get_file_options() |
| src/compression.rs | Changed get_file_options() return type lifetime from '_ to 'static to support caching |
| src/build_context.rs | Added zip_mtime() function; updated all 5 WheelWriter::new() call sites to compute and pass file_options instead of compression |
| fn zip_mtime() -> DateTime { | ||
| let res = env::var("SOURCE_DATE_EPOCH") | ||
| .context("") // Only using context() to unify the error types | ||
| .and_then(|epoch| { | ||
| let epoch: i64 = epoch.parse()?; | ||
| let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?; | ||
| let dt = DateTime::try_from(dt)?; | ||
| Ok(dt) | ||
| }); | ||
|
|
||
| res.unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
The old mtime() method in wheel_writer.rs enforced a minimum date of 1980-01-01 (the earliest timestamp a zip file can represent), but this logic is missing in the new zip_mtime() function. This could cause issues if SOURCE_DATE_EPOCH is set to a date before 1980-01-01.
The old implementation included:
let min_dt = time::Date::from_calendar_date(1980, time::Month::January, 1)
.unwrap()
.midnight()
.assume_offset(time::UtcOffset::UTC);
let dt = dt.max(min_dt);Consider adding this validation to zip_mtime():
fn zip_mtime() -> DateTime {
let res = env::var("SOURCE_DATE_EPOCH")
.context("")
.and_then(|epoch| {
let epoch: i64 = epoch.parse()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?;
// Ensure the date is at least 1980-01-01 (earliest zip timestamp)
let min_dt = time::Date::from_calendar_date(1980, time::Month::January, 1)
.unwrap()
.midnight()
.assume_offset(time::UtcOffset::UTC);
let dt = dt.max(min_dt);
let dt = DateTime::try_from(dt)?;
Ok(dt)
});
res.unwrap_or_default()
}There was a problem hiding this comment.
zip::DateTime::try_from() will return an error if the timestamp is before Jan 1 1980. Also zip::DateTime::default() returns a datetime of Jan 1 1980 so, together with the unwrap_or_default(), this code can never return a timestamp earlier than Jan 1 1980.
| let res = env::var("SOURCE_DATE_EPOCH") | ||
| .context("") // Only using context() to unify the error types | ||
| .and_then(|epoch| { | ||
| let epoch: i64 = epoch.parse()?; | ||
| let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?; | ||
| let dt = DateTime::try_from(dt)?; | ||
| Ok(dt) | ||
| }); | ||
|
|
||
| res.unwrap_or_default() |
There was a problem hiding this comment.
[nitpick] The empty context string context("") is unusual and appears to be used solely to unify error types. Consider using a more descriptive error message or using .ok() followed by .and_then() to avoid needing a context at all.
For example:
fn zip_mtime() -> DateTime {
env::var("SOURCE_DATE_EPOCH")
.ok()
.and_then(|epoch| {
let epoch: i64 = epoch.parse().ok()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch).ok()?;
DateTime::try_from(dt).ok()
})
.unwrap_or_default()
}| let res = env::var("SOURCE_DATE_EPOCH") | |
| .context("") // Only using context() to unify the error types | |
| .and_then(|epoch| { | |
| let epoch: i64 = epoch.parse()?; | |
| let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?; | |
| let dt = DateTime::try_from(dt)?; | |
| Ok(dt) | |
| }); | |
| res.unwrap_or_default() | |
| env::var("SOURCE_DATE_EPOCH") | |
| .ok() | |
| .and_then(|epoch| { | |
| let epoch: i64 = epoch.parse().ok()?; | |
| let dt = time::OffsetDateTime::from_unix_timestamp(epoch).ok()?; | |
| DateTime::try_from(dt).ok() | |
| }) | |
| .unwrap_or_default() |
There was a problem hiding this comment.
I'm not convinced that writing .ok()? everywhere is better than just ? but the code does the same thing either way so 🤷
We can store the file options directly after calculating the compression options and mtime once since they will never change.